Skip to content

Comments

[jaspLearnStats] Fix issue #3107 (second issue - plot sizing): Increase the default...#150

Open
sisyphus-jasp wants to merge 1 commit intojasp-stats:masterfrom
sisyphus-jasp:fix-sisyphus-1771946262
Open

[jaspLearnStats] Fix issue #3107 (second issue - plot sizing): Increase the default...#150
sisyphus-jasp wants to merge 1 commit intojasp-stats:masterfrom
sisyphus-jasp:fix-sisyphus-1771946262

Conversation

@sisyphus-jasp
Copy link
Contributor

Summary

Fixes: https://github.com/jasp-stats/INTERNAL-jasp/issues/3107

Summary: Fix Issue #3107 - Mosaic Plot Sizing

What was wrong

The mosaic plots in the Effect Sizes analysis (phi effect size) were using a default width that was too small. The plots were created without an explicit width parameter, resulting in a default width derived from height * aspectRatio (500 * 1 = 500).

What was changed

Modified /workspace/R/LSeffectSizes.R in the .tsPhiPlot function to add explicit width = 650 to three createJaspPlot calls:

  1. Line 1035: phiCombinedPlot - combined population and simulation plot
  2. Line 1048: phiPopulationPlot - population distribution plot
  3. Line 1060: phiSimulationPlot - simulation distribution plot

The new width of 650 represents a 30% increase from the previous default of 500 (500 * 1.3 = 650).

Verification

  • R file loads without syntax errors
  • No unit tests exist in this module to run
  • The change is minimal and focused on the specific issue

Implementation Plan

Plan: Increase default width of mosaic plots by 30%

Root Cause

The phi effect size plots (mosaic plots) in /workspace/R/LSeffectSizes.R are created without specifying an explicit width parameter. Currently they use:

  • height = 500
  • aspectRatio = 1

This results in a default width of ~500 (height * aspectRatio). The issue requests increasing this by 30%, so the new width should be 650.

Location

File: /workspace/R/LSeffectSizes.R
Function: .tsPhiPlot (lines 1022-1072)

Changes Needed

Add width = 650 to three createJaspPlot calls:

  1. Line 1035: phiCombinedPlot - combined population and simulation plot
  2. Line 1048: phiPopulationPlot - population distribution plot
  3. Line 1060: phiSimulationPlot - simulation distribution plot

All currently have height = 500, aspectRatio = 1. Adding width = 650 increases width by 30% (500 → 650).

Test Impact

  • Existing tests may have snapshot expectations based on the old plot dimensions
  • Tests will likely need snapshot updates if they compare exact plot dimensions
  • The functional behavior of the plots remains the same, just larger

Test Results

Test Run Result
Baseline (pre-fix) Could not parse test summary
Post-fix Could not parse test summary
Upstream CI ed4f0df -- CI: passing

@FBartos FBartos requested a review from vandenman February 24, 2026 15:25
Copy link
Contributor

@vandenman vandenman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the problem that it uses too many breaks and tick labels? Using fewer by default would also resolve the issue. Or am I looking at the wrong thing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants